tests/src/tests/defuse/tokens/nep245/mt_transfer_resolve_gas.rs (1)

278-291: Test coverage for binary_search_max is good but doesn't cover edge case.

The test validates all values from 0 to 99, which is comprehensive. However, note that this test only covers cases where at least test(0) succeeds (since limit ranges from 0 to 99 and test(x) succeeds when x <= limit).

Consider adding a test case where all values fail to ensure the underflow issue mentioned in mod.rs is covered:

✨ Additional test case

// Test case where all values fail
let test_all_fail = |_x| async { Err(anyhow::anyhow!("always fail")) };
assert_eq!(binary_search_max(0, max, test_all_fail).await, None);

sandbox/Cargo.toml (1)

21-21: Add libc to workspace dependencies for consistency.

The libc crate is used in the sandbox code (libc::atexit in sandbox/src/lib.rs) but is specified with a direct version instead of using the workspace = true pattern like all other dependencies. Declare it in the workspace [dependencies] section to maintain consistency across the codebase.

sandbox/src/lib.rs (1)

115-140: Potential cascading test failures on poisoned mutex.

If a test panics while the sandbox fixture is being created (before the lock is released), subsequent tests will fail with a panic on line 126's .unwrap() due to mutex poisoning. Consider recovering from a poisoned mutex or documenting this behavior.

That said, for test infrastructure where a panic during setup indicates a fundamental issue, this is often acceptable.

♻️ Optional: Handle poisoned mutex gracefully

     let (sandbox_arc, root_account) = mutex
         .lock()
-        .unwrap()
+        .unwrap_or_else(|poisoned| poisoned.into_inner())
         .as_ref()
         .map(|shared| (shared.sandbox.clone(), shared.root.clone()))
         .unwrap();

tests/src/tests/defuse/tokens/nep245/mt_deposit_resolve_gas.rs (1)

74-85: Potential underflow in account name length calculation.

If root_id_len >= 63, the subtraction TARGET_LEN - 1 - root_id_len will underflow (wrap around to a large value), causing the subaccount creation to likely fail or behave unexpectedly.

In practice, the sandbox root ID (test or similar) is short, so this won't trigger. However, adding a guard would make the code more robust.

♻️ Optional: Add a guard for name length calculation

         TokenIdGenerationMode::Medium => {
             // Use a 64-char named account: {name}.{root_id} = 64 chars total
             const TARGET_LEN: usize = 64;
             let root_id_len = env.root().id().as_str().len();
             // name_len + 1 (dot) + root_id_len = TARGET_LEN
+            assert!(
+                root_id_len < TARGET_LEN - 1,
+                "root_id too long for medium mode"
+            );
             let name_len = TARGET_LEN - 1 - root_id_len;

Configuration used: defaults

Review profile: CHILL

Plan: Pro

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

tests/src/tests/defuse/tokens/nep245/mod.rs (1)

17-28: Potential underflow when mid == 0 and test fails.

If lo is 0 and test(0) returns Err, line 25 computes hi = mid - 1 which causes unsigned integer underflow (0 - 1 wraps to usize::MAX). This would cause the loop to continue with invalid bounds.

While current usage with min_token_count = 1 avoids this, the function could fail unexpectedly if called with low = 0.

🐛 Proposed fix to handle underflow

         match test(mid).await {
             Ok(()) => {
                 best = Some(mid);
                 lo = mid + 1; // success -> try higher
             }
             Err(_) => {
+                if mid == 0 {
+                    break;
+                }
                 hi = mid - 1; // failure -> try lower
             }
         }

tests/src/tests/defuse/tokens/nep245/mt_transfer_resolve_gas.rs (1)

395-424: Consider consolidating log size calculation helpers.

There's some duplication between calculate_log_sizes (lines 395-424) and validate_mt_batch_transfer_log_size (lines 83-106). Both construct similar MtTransferEvent structures for size calculation.

♻️ Suggested consolidation

You could refactor to have a single helper that returns both sizes:

-fn validate_mt_batch_transfer_log_size(
-    sender_id: &AccountId,
-    receiver_id: &AccountId,
-    token_ids: &[String],
-    amounts: &[u128],
-) -> anyhow::Result<usize> {
-    let mt_transfer_event = MtEvent::MtTransfer(Cow::Owned(vec![MtTransferEvent {
-        authorized_id: None,
-        old_owner_id: Cow::Borrowed(receiver_id),
-        new_owner_id: Cow::Borrowed(sender_id),
-        token_ids: Cow::Owned(token_ids.to_vec()),
-        amounts: Cow::Owned(amounts.iter().copied().map(U128).collect()),
-        memo: Some(Cow::Borrowed("refund")),
-    }]));
-
-    let longest_transfer_log = mt_transfer_event.to_nep297_event().to_event_log();
-
-    anyhow::ensure!(
-        longest_transfer_log.len() <= TOTAL_LOG_LENGTH_LIMIT,
-        "transfer log will exceed maximum log limit"
-    );
-
-    Ok(longest_transfer_log.len())
-}
+fn validate_mt_batch_transfer_log_size(
+    sender_id: &AccountId,
+    receiver_id: &AccountId,
+    token_ids: &[String],
+    amounts: &[u128],
+) -> anyhow::Result<usize> {
+    let (_, refund_size) = calculate_log_sizes(sender_id, receiver_id, token_ids, amounts);
+    anyhow::ensure!(
+        refund_size <= TOTAL_LOG_LENGTH_LIMIT,
+        "transfer log will exceed maximum log limit"
+    );
+    Ok(refund_size)
+}

sandbox/src/lib.rs (1)

92-94: Consider returning Result from fast_forward instead of panicking.

Since this is a public API, avoid unwrap() so callers can handle errors with context; good to lock this in now while the method is new.

♻️ Suggested signature change

-    pub async fn fast_forward(&self, blocks: u64) {
-        self.sandbox.fast_forward(blocks).await.unwrap();
-    }
+    pub async fn fast_forward(&self, blocks: u64) -> anyhow::Result<()> {
+        self.sandbox.fast_forward(blocks).await.map_err(Into::into)
+    }

tests/src/tests/defuse/tokens/nep245/mt_deposit_resolve_gas.rs (1)

239-248: Ensure resolve receipt is always validated.

The current check only validates when exactly 2 receipts exist. If there are more or fewer, the resolve callback validation is silently skipped. The second test at lines 423-427 uses the correct pattern with assert!(defuse_outcomes.len() >= 2, ...).

🔧 Proposed fix for consistent validation

-    if defuse_outcomes.len() == 2 {
-        let resolve_outcome = defuse_outcomes[1].clone();
-        let resolve_result = resolve_outcome.into_result();
-        assert!(
-            resolve_result.is_ok(),
-            "CRITICAL: mt_resolve_deposit callback failed for token_count={token_count}! \
-            This indicates insufficient gas allocation in the contract. Error: {:?}",
-            resolve_result.err()
-        );
-    }
+    anyhow::ensure!(
+        defuse_outcomes.len() >= 2,
+        "Expected at least 2 defuse receipts, got {}",
+        defuse_outcomes.len()
+    );
+    let resolve_outcome = defuse_outcomes[1].clone();
+    let resolve_result = resolve_outcome.into_result();
+    assert!(
+        resolve_result.is_ok(),
+        "CRITICAL: mt_resolve_deposit callback failed for token_count={token_count}! \
+        This indicates insufficient gas allocation in the contract. Error: {:?}",
+        resolve_result.err()
+    );

tests/src/tests/defuse/tokens/nep245/mt_deposit_resolve_gas.rs (1)

336-336: Use expect() with a descriptive message.

If binary_search_max returns None (meaning even min_token_count=1 failed), the unwrap() will panic without context, making debugging harder.

♻️ Proposed fix

-    let max_deposited_count = max_deposited_count.unwrap();
+    let max_deposited_count = max_deposited_count
+        .expect("Failed to find any successful token count - even min_token_count=1 failed");

nep245/src/events.rs (2)

44-49: Conservative overhead estimation may over-reject valid events.

The function returns max(REFUND_EXTRA_BYTES, memo_length), but when a memo already exists, the actual overhead is the difference between refund memo size and existing memo size, not the full REFUND_EXTRA_BYTES.

For example, if memo = Some("refu") (4 chars):

• Original includes ,"memo":"refu" (~13 bytes)
• Refund includes ,"memo":"refund" (~16 bytes)
• Actual overhead: ~3 bytes, not 16

This conservative approach is safe (won't cause runtime failures) but may reject some valid events unnecessarily. If precision matters, consider computing the actual delta.

---

136-157: Consider using this accurate method in validate_with_refund_overhead for transfers.

This method computes the exact refund log size, while validate_with_refund_overhead uses the heuristic compute_refund_overhead. For MtTransfer events, using this accurate method would avoid over-rejecting valid events.

The current approach is simpler and applies uniformly to all event types, so this is just a consideration if precision becomes important.

tests/src/tests/defuse/tokens/nep245/mt_deposit_resolve_gas.rs (2)

208-221: Always validate resolve receipt presence.

The check only runs when defuse_outcomes.len() == 2; if the count differs, resolve failures can be skipped. Assert >= 2 and always inspect the second receipt.

✅ Suggested fix

-    if defuse_outcomes.len() == 2 {
-        let resolve_outcome = defuse_outcomes[1].clone();
-        let resolve_result = resolve_outcome.into_result();
-        assert!(
-            resolve_result.is_ok(),
-            "CRITICAL: mt_resolve_deposit callback failed for token_count={token_count}! \
-            This indicates insufficient gas allocation in the contract. Error: {:?}",
-            resolve_result.err()
-        );
-    }
+    anyhow::ensure!(
+        defuse_outcomes.len() >= 2,
+        "Expected at least 2 defuse receipts, got {}",
+        defuse_outcomes.len()
+    );
+    let resolve_outcome = defuse_outcomes[1].clone();
+    let resolve_result = resolve_outcome.into_result();
+    assert!(
+        resolve_result.is_ok(),
+        "CRITICAL: mt_resolve_deposit callback failed for token_count={token_count}! \
+        This indicates insufficient gas allocation in the contract. Error: {:?}",
+        resolve_result.err()
+    );

---

351-352: Fix typo in test name: “desposit” → “deposit”.

✏️ Proposed fix

-async fn mt_desposit_resolve_can_handle_large_blob_value_returned_from_notification() {
+async fn mt_deposit_resolve_can_handle_large_blob_value_returned_from_notification() {

nep245/src/events.rs (1)

16-17: Duplicate constant should be imported from defuse-near-utils.

As flagged in previous reviews, TOTAL_LOG_LENGTH_LIMIT is already exported from near-utils/src/event.rs. Import it instead of redefining:

use defuse_near_utils::TOTAL_LOG_LENGTH_LIMIT;

tests/src/tests/defuse/tokens/nep245/mt_transfer_resolve_gas.rs (2)

320-327: Consider documenting the token count magic numbers.

The generation of 65 max-length tokens plus 2 specific trigger tokens appears carefully calculated to hit the boundary condition. A brief comment explaining why these specific counts/lengths trigger the refund-log-exceeds-limit condition would improve maintainability:

// 65 max-length tokens keep transfer log under limit but push refund log over
// The 2 additional tokens ensure the refund event exceeds TOTAL_LOG_LENGTH_LIMIT
let token_ids: Vec<String> = (1..=65)
    ...

---

380-384: String-based error matching may be fragile.

If the error message in the contract changes, this assertion will silently pass with incorrect behavior. Consider also checking for a specific error type/code if the contract API supports it, or add a comment noting this coupling.

// NOTE: This string must match the error message in defuse contract's mt_batch_transfer_call
assert!(
    result_str.contains("too many tokens: refund log would exceed protocol limit"),
    ...
);

Makefile.toml (1)

211-213: run-tests-long still applies --features=long to the entire workspace.

This will fail for workspace crates that don’t define the feature. Scope the run to packages that declare long, or run per-crate with -p/--package filters.

#!/bin/bash
set -euo pipefail

python - <<'PY'
import glob, pathlib, tomllib

root = pathlib.Path("Cargo.toml")
data = tomllib.loads(root.read_text())
members = data.get("workspace", {}).get("members", [])

def expand(member):
    if any(ch in member for ch in "*?[]"):
        return [pathlib.Path(p) for p in glob.glob(member)]
    return [pathlib.Path(member)]

missing = []
for m in members:
    for path in expand(m):
        cargo = path / "Cargo.toml"
        if not cargo.exists():
            continue
        pkg = tomllib.loads(cargo.read_text())
        features = pkg.get("features", {})
        if "long" not in features:
            missing.append(str(cargo))

print("\n".join(sorted(set(missing))) or "All workspace packages define feature 'long'")
PY

tests/contracts/multi-token-receiver-stub/src/lib.rs (1)

13-19: Guard stub_return_bytes length and avoid U128 → u64 truncation/panic.

The stub trusts the input length and uses unchecked casts; this can allocate huge buffers or panic before the intended limit checks.

🛠 Suggested guardrails

 pub extern "C" fn stub_return_bytes() {
     if let Some(input) = near_sdk::env::input() {
         if input.len() >= 8 {
-            let len = u64::from_le_bytes(input[..8].try_into().unwrap()) as usize;
-            let bytes = vec![0xf0u8; len];
+            const MAX_RETURN_BYTES: usize = 4 * 1024 * 1024;
+            let len = u64::from_le_bytes(input[..8].try_into().unwrap());
+            let len = usize::try_from(len)
+                .ok()
+                .filter(|l| *l <= MAX_RETURN_BYTES)
+                .unwrap_or_else(|| env::panic_str("return length too large"));
+            let bytes = vec![0xf0u8; len];
             near_sdk::env::value_return(&bytes);
         }
     }
 }
@@
-            MTReceiverMode::ReturnBytes(len) => Promise::new(env::current_account_id())
-                .stub_return_bytes(len.0.try_into().unwrap())
-                .into(),
+            MTReceiverMode::ReturnBytes(len) => {
+                let len = u64::try_from(len.0)
+                    .unwrap_or_else(|_| env::panic_str("ReturnBytes length overflows u64"));
+                Promise::new(env::current_account_id())
+                    .stub_return_bytes(len)
+                    .into()
+            }

Also applies to: 99-101

tests/src/tests/defuse/tokens/nep245/mod.rs (1)

7-26: Handle mid == 0 to avoid usize underflow.

When the test fails at mid == 0, hi = mid - 1 wraps to usize::MAX.

🛠 Safer update for the Err branch

             Err(_) => {
-                hi = mid - 1; // failure -> try lower
+                if mid == 0 {
+                    break;
+                }
+                hi = mid - 1; // failure -> try lower
             }

#!/bin/bash
# Find call sites to confirm whether low can be 0
rg -n --type=rs -C3 "binary_search_max\s*\(" tests

tests/src/tests/defuse/tokens/nep245/mt_transfer_resolve_gas.rs (1)

341-345: Reuse the shared log-length limit constant.

Line 344 uses TOTAL_LOG_LENGTH_LIMIT; to avoid drift, import it from defuse::nep245 and remove the local constant (Line 26). This was raised before and still applies.

♻️ Proposed fix

 use defuse::{
     core::token_id::{TokenId, nep245::Nep245TokenId},
-    nep245::{MtEvent, MtTransferEvent},
+    nep245::{MtEvent, MtTransferEvent, TOTAL_LOG_LENGTH_LIMIT},
 };
@@
-const TOTAL_LOG_LENGTH_LIMIT: usize = 16384;

nep245/src/events.rs (1)

3-5: Avoid a duplicate TOTAL_LOG_LENGTH_LIMIT definition.

Line 16 redefines a constant already exported by defuse-near-utils. Re-export it here instead of redefining to keep a single source of truth.

♻️ Proposed fix

-use near_sdk::{
+use near_sdk::{
     AccountIdRef, AsNep297Event, FunctionError, json_types::U128, near, serde::Deserialize,
 };
@@
-/// NEAR protocol limit for log messages (16 KiB)
-pub const TOTAL_LOG_LENGTH_LIMIT: usize = 16384;
+/// NEAR protocol limit for log messages (16 KiB)
+pub use defuse_near_utils::TOTAL_LOG_LENGTH_LIMIT;

Also applies to: 16-17

tests/src/tests/defuse/tokens/nep245/mt_deposit_resolve_gas.rs (2)

212-221: Always validate resolve receipt presence.

Line 212 only checks when defuse_outcomes.len() == 2; if the count differs, resolve failures can be silently skipped. This was flagged earlier and is still present.

✅ Suggested fix

-    if defuse_outcomes.len() == 2 {
-        let resolve_outcome = defuse_outcomes[1].clone();
-        let resolve_result = resolve_outcome.into_result();
-        assert!(
-            resolve_result.is_ok(),
-            "CRITICAL: mt_resolve_deposit callback failed for token_count={token_count}! \
-            This indicates insufficient gas allocation in the contract. Error: {:?}",
-            resolve_result.err()
-        );
-    }
+    anyhow::ensure!(
+        defuse_outcomes.len() >= 2,
+        "Expected at least 2 defuse receipts, got {}",
+        defuse_outcomes.len()
+    );
+    let resolve_outcome = defuse_outcomes[1].clone();
+    let resolve_result = resolve_outcome.into_result();
+    assert!(
+        resolve_result.is_ok(),
+        "CRITICAL: mt_resolve_deposit callback failed for token_count={token_count}! \
+        This indicates insufficient gas allocation in the contract. Error: {:?}",
+        resolve_result.err()
+    );

---

351-352: Fix test name typo (“desposit” → “deposit”).

The misspelling in the test name (Line 351) reduces clarity/discoverability. This is a repeat from earlier.

✏️ Proposed fix

-async fn mt_desposit_resolve_can_handle_large_blob_value_returned_from_notification() {
+async fn mt_deposit_resolve_can_handle_large_blob_value_returned_from_notification() {